Root certs are not used on Windows #67
Conversation
…stead was defaulting to the gocertifi library. This bug prevents users from adding trusted roots and verifying signatures using those roots
|
I see these tests failing on OSX, I will address this by first attempting to read the store using x509.SystemCertPool() call to basically test if I am on Windows or OSX and proceed from there |
…at are called depa nding on operating system
…ions reading the system root store into two files for unix systems and windows systems.
|
Apologies for the bad initial request. Testing was passing locally, but failed in travis due to OSX throwing errors with the windows specific syscall methods. Tests are now passing with the go 1.12 and 1.x configurations and failing on master. This is consistent with the state of the project before this fix. |
|
Thanks for opening the PR. We have one other PR that is also failing CI for similar reasons (I think). Let us take a look at that and circle back. |
|
It isn't just the other PR that is failing CI, the whole project failed for the |
|
@ptoomey3, the failing Travis tests are not due to any issue with the changes contained in this PR. Unit tests are passing on the 1.12 and 1.14 golang configurations in Travis. I opened an issue to address the failing Travis tests here, and I'll happily help troubleshoot this issue. Respectfully request that you ignore the current state of Travis testing to address the issue with Windows Cert store. The results of these tests match the latest commit to this project. |
|
Thanks for opening the issue. Someone on our team plans to take a look this later this week. |
|
We will chat about it on our team call today. |
|
Thank you @lgarron! All unit tests passing |
|
Can I get a quick update on this? It's been a few months |
There was a problem hiding this comment.
Many thanks for the patience on a review! My initial thought is, in principle, I like the idea of this change so that roots in the System store are considered as valid anchors.
However, as discussed in the golang/go#16736 (comment), enumerating the Windows Machine/Root trust store is problematic in that it is dynamically populated. So, even though a certificate is theoretically in this trust store, it won't be in the enumeration unless the authroot STL has been pulled down. This is why Go's pull request to implement this was reverted.
With this change, it seems possible or likely that root certificates that were once being returned will now be missing.
Perhaps this can be implemented this as a union of gocertifi's NSS roots and the Windows store and de-duplicate the results. That way roots that were returned previously will continue to be returned, and anything new in the Windows trust store will be added.
| if err != nil { | ||
| return errors.Wrap(err, "Failed to get root store name") | ||
| } | ||
| storeHandle, err := syscall.CertOpenSystemStore(0, storeName) |
There was a problem hiding this comment.
I don't see a call to CertCloseStore anywhere, so I think this is leaking HCERTSTORE handles.
There was a problem hiding this comment.
I added a defered call to CertCloseStore to resolve this.
The chain in question is defined in RFC5652 as a CertificateSet, which is a `SET OF` certs: - https://tools.ietf.org/html/rfc5652#section-5.1 - https://tools.ietf.org/html/rfc5652#section-10.2.3 Go 1.15 introduced a change that sorts such sets: https://go.googlesource.com/go/+/f0cea848679b8f8cdc5f76e1b1e36ebb924a68f8 Our tests were implicitly relying on the sets to stay sorted, but the right thing to do at this point is update our tests.
…e Certificate Store when completed enumarating trusted roots on Windows
…oots to this pool. Duplicate certs are automatically dropped
…n into BugFix/RootCerts_windows
|
Thanks for the review @vcsjones. I have addressed your concern about unreliable enumeration of the windows root store in the following way. I hope this addresses your concerns sufficiently. There is still a chance that trusted certificates that are not in the Mozilla trusted roots bundle will not be enumerated and verification will fail, but there is now a baseline of trusted certificates which will always populate. |
|
Been a few months. Can I get a checkup on this? |
| return errors.Wrap(err, "Failed to parse root store") | ||
| } | ||
|
|
||
| for _, ident := range idents { |
There was a problem hiding this comment.
I think we should also continue to fallback to gocertifi.CACerts() if we cannot open the SystemCertPool on macOS / Linux.
While the fallback path was primarily there for Windows support, macOS could still have a non-nil err if opening the root store failed.
There was a problem hiding this comment.
Sounds good, I’ll go ahead and get to work on implementing this
The implementation of the verify command fails to read from the system root store on Windows. This was likely done due to issues with the x509 library which fails to parse the root store as detailed in Issue #16736 of the golang crypto library. Windows instead was defaulting to the gocertifi library, which prevents users from adding or removing trusted roots, and verifying signatures against those roots.
In the golang crypto issue referenced before, there is a workaround which enables us to parse the root store and verify signatures against these roots. I have adopted this fix to this project.
All tests passing locally.
Thanks to @ndouthit for help in troubleshooting this issue.